Skip to content

Conversation

vcjana
Copy link
Contributor

@vcjana vcjana commented Oct 13, 2025

Motivation and Context

It is reported that the S3 GetBucketLocation operation is marked as deprecated in the AWS S3 documentation, but the Rust SDK doesn't reflect this deprecation with a compiler warning.
The AWS documentation recommends using HeadBucket instead to determine a bucket's region.

Description

This PR adds the Smithy @deprecated trait to the GetBucketLocation operation in the S3Decorator, which generates a Rust #[deprecated] attribute in the SDK.

Changes:

  • S3Decorator.kt: Added logic to apply DeprecatedTrait to GetBucketLocation operation
  • S3DecoratorTest.kt: Added comprehensive unit and integration tests to verify:
  • GetBucketLocation has the DeprecatedTrait applied
  • Other S3 operations (HeadBucket, CreateBucket) are NOT deprecated
  • Generated Rust code includes the #[deprecated] attribute
  • Deprecation message includes HeadBucket recommendation and AWS documentation link

Deprecation Message:

Use HeadBucket operation instead to determine the bucket's region. 
For more information, see https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html

Testing

  • Unit tests verify the trait is applied correctly to the model
  • Integration tests verify the generated Rust code compiles with the deprecation attribute
  • Manual verification completed

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.
  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

- Add DeprecatedTrait to GetBucketLocation operation in S3Decorator
- Include message recommending HeadBucket as the preferred alternative
- Add comprehensive unit tests to verify deprecation behavior
- Add integration test to verify generated Rust code includes #[deprecated] attribute

This addresses customer feedback that GetBucketLocation is marked as
deprecated in AWS documentation but not in the Rust SDK.
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, couple small comments, but no major blocker. Lets hold off on merging to see if we can get this change into the actual S3 model though.

- Changed createDeprecatedTrait() to take message as parameter
- Updated deprecatedOperations from Set to Map for message storage
- Removed integration test per code review feedback
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left minor comments.

Comment on lines +130 to +131
@Test
fun `Other S3 operations do not have DeprecatedTrait`() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also remove this test (as well as relevant operations from the test model), as long as we are testing GetBucketLocation to be annotated for deprecation. Since there are many other S3 operations in the real world, the fact this test passed doesn't necessarily guarantee those operations are without deprecated annotations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general tip, if these are only format changes, I'd keep the one in main and not include it in the PR (as long as precommit hook is ok). It'll be hard to distinguish between the real changes vs. format changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants